613 Fix CqlComparer for tuples #614
Open
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…mpiler' into 613-fix-integration-test---systemargumentexception-cannot-generate-a-hash-code-for-valuetuple3-arg_paramname_name
…mpiler' into 613-fix-integration-test---systemargumentexception-cannot-generate-a-hash-code-for-valuetuple3-arg_paramname_name
…m to work in Release build
baseTwo
changed the base branch from
develop-2.0
to
615-change-unit-tests-to-compile-and-run-via-assemblycompiler
…mpiler' into 613-fix-integration-test---systemargumentexception-cannot-generate-a-hash-code-for-valuetuple3-arg_paramname_name
Base automatically changed from
615-change-unit-tests-to-compile-and-run-via-assemblycompiler
to
develop-2.0
October 22, 2024 14:12
ewoutkramer
reviewed
Oct 23, 2024
ewoutkramer
reviewed
Oct 23, 2024
ewoutkramer
reviewed
Oct 23, 2024
ewoutkramer
reviewed
Oct 23, 2024
| { | ||
| 0 => Comparer<TValue>.Default.Compare(x.Value, y.Value), | ||
| 0 => cqlComparer.Compare(x.Value, y.Value, precision), |
There was a problem hiding this comment.
Was this a bug, the fact that precision wasn't passed on to the compare?
There was a problem hiding this comment.
The .NET Comparer wasn't capable of taking a precision. This was replaced with the ICqlComparer, it would make sense to use precision on KeyValuePair's Value property.
ewoutkramer
reviewed
Oct 23, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fix for
Work
TupleComparerwithCqlTupleTypeComparer. The new comparer compares value tuples where the first parameter is of typeCqlTupleMetadataand uses the metadata to compare items.TupleBaseTypeComparercannot be removed yet, since some unit tests still execute against directly compiled LambdaExpressions, instead of going through the C# code writer and assembly compiled.CqlTupleMetadatahad to be moved to a more base project and was moved fromHl7.Cql.Runtime' toHl7.Cql.Primitives`TypeExtensionshad to move to a more base project too, and was moved fromHl7.Cql.CompilertoHl7.Cql.Primitives. The extensions for this particularTypeExtensionsis for types related to CQL.TypeExtensionsTestsIntegration Runner
While working on this PR, I also looked at the integration tests for CMS measure. I found they were flaky, due to the libraries sometimes failing to create. This was due to the CqlTupleMetadata calculating the hashcode in the constructor, and that used Hasher.Instance which uses MD5 to hash a string. For some strange reason, a NullReferenceException is sometimes thrown deep inside the stack trace. I changed the
CqlTupleMetadatato do hash calculation lazily.I also discovered that the
KeyValuePairComparer<TKey,TValue>comparer fails whenTValueis anobjectwhen doing a compare. An error is thrown stating that 'object' is not comparable. I fixed this by using the ICqlComparer on key and value instead.Test pass rate increased from 68% to 72%